-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
Please review my patch! ;-) |
func GetLog(w http.ResponseWriter, r *http.Request) { | ||
repo := r.URL.Query().Get(":name") | ||
ref := r.URL.Query().Get("ref") | ||
total, _ := strconv.Atoi(r.URL.Query().Get("total")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you are ignoring this error. What will happens if total
can't be converted to int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total will be 0 and GetLog will return only one entry :-P
But yes, a bad request would be better.
@marcelometal sorry for the delay. See the comments above. |
b, err := json.Marshal(logs) | ||
if err != nil { | ||
err := fmt.Errorf("Error when trying to obtain log for ref %s of repository %s (%s).", ref, repo, err) | ||
http.Error(w, err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a json marshalling error be a bad request? I believe http.StatusInternalServerError fits better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I will send a patch to fix all others handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, thanks! :)
Return git log for a given repository.
Done! Please review again ;-) |
LGTM @andrewsmedina feel free to merge it when you're good :) |
@marcelometal thank you! |
Return git log for a given repository.